feat(review-fix-loop): deliver and evaluate standalone local_commit workflow - #111
Merged
Conversation
…orkflow ## Summary - Add `scripts/local_commit.py`'s `run_local_commit(...)`, composing the merged contract (#96), local-execution (#97), and reviewer-orchestration (#98) leaves into the actual end-to-end `local_commit` loop design's "Workflow" section describes: resolve, establish evidence, review, decide, fix, validate and commit, invalidate and repeat, publish (never, under `local_commit`), and return one schema-valid terminal result. - Enforce the invocation fix-cycle budget, commit selected fixes in isolated attempts, promote only a converged candidate, and detect the candidate-visible non-convergence stops (`expanding_findings`, `oscillation`, `repeated_failed_attempt`, `cycle_budget_exhausted`) without second-guessing a finding that simply survives one fix within budget. - Add `references/local-commit.md` documenting the composition, the three host-boundary ports (`reviewer`, `decide`, `apply_fix`), and every terminal state this module returns. - Add `scripts/tests/test_local_commit.py`: 18 end-to-end tests against real temporary Git repositories (no mocked Git state) covering immediate convergence, one and multiple fix cycles, budget exhaustion, a validation failure that is unavailable/untractable/tractable, a declined finding and a scope-expanding fix (both surfaced as operator input), expanding and oscillating finding sets, repeated failed attempts, interrupted-attempt recovery, an already-held candidate lock, an attempted reviewer mutation, and a host without fresh-subagent support. - Update `SKILL.md` to document the new entry point and record the changelog entry for today, backfilling the prior day's topmost entry's SHA. ## Why - Closes #99, the only unblocked child of epic #95: a standalone, evaluated `local_commit` workflow that reviews, fixes, validates, re-reviews, and converges without remote publication, usable by a future caller without caller-specific knowledge. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…und source ahead/behind
## Summary
- `_State.record_finding_disposition` replaces any existing top-level entry
for a finding_id before appending a new one, in both `finding_dispositions`
and `unresolved_or_deferred_findings`, instead of letting a declined
disposition from one pass (or a resumed checkpoint) coexist with a later
`selected` entry for the same finding once it is actually fixed. Wired into
every disposition call site (resume reconstruction, the live decline path,
and the live commit-success path).
- `_checkpoint_source_status`/`_terminal_source_status` report `source:
{status: "bound", ...}` with real ahead/behind counts (via `git rev-list
--count`) whenever `candidate.source_binding` is present, instead of
unconditionally reporting `unavailable` even when a read-only source
binding exists; `_minimal_blocked_result` (the pre-lock `candidate_busy`
path) now takes `repo` so it can report the same thing.
- Add regression tests reproducing both defects directly (a finding declined
then resumed-and-fixed ends up with exactly one, current disposition and
is no longer reported as unresolved; a `local_commit` invocation carrying
a `source_binding` reports `bound` with correct ahead/behind counts) plus
a control test confirming the no-binding path is unaffected.
## Why
- Closes the first review-code-change pass on #99: a `blocking` correctness
finding (self-contradictory disposition data reachable through this
module's own public `resume_checkpoint` parameter) and a
`strong_recommendation` finding (the terminal-result contract's own named
"local ahead/behind state ... when a source is bound" requirement was
silently unimplemented).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Delivers and evaluates the standalone
review-fix-looplocal_commitworkflow: the actual end-to-end loopdesign/review-fix-loop.md's "Workflow" section describes (steps 1 "Resolve" through 9 "Return"), composing the three already-merged children of epic #95 without reimplementing any of their internals.skills/review-fix-loop/scripts/local_commit.py—run_local_commit(...)composes:validate.py(Define review-fix-loop invocation, checkpoint, and terminal-result contracts #96) for invocation/checkpoint/terminal-result contract validation;local_execution.py(Implement review-fix-loop local locking, isolated attempts, and recovery #97) for candidate locking, isolated attempt worktrees, checkpointing, verified fast-forward-only promotion, and interrupted-attempt recovery;reviewer_orchestration.py(Implement review-fix-loop reviewer isolation and complete-review orchestration #98) for fixed lens resolution, raw-result evaluation/binding, mutation detection, and deterministic finding ordering.local_commit.expanding_findings,oscillation,repeated_failed_attempt,cycle_budget_exhausted) without second-guessing a finding that simply survives one fix within budget (seereferences/local-commit.mdfor why that specific case is deliberately not an automatic early stop).unpushed_commits) andoperator_actionevidence for every non-converged stop.skills/review-fix-loop/references/local-commit.mddocuments the composition, the three host-boundary ports (reviewer,decide,apply_fix), and every terminal state this module returns.skills/review-fix-loop/scripts/tests/test_local_commit.py— 21 end-to-end tests against real temporary Git repositories (no mocked Git state): immediate convergence, one and multiple fix cycles, budget exhaustion, a validation failure that is unavailable/untractable/tractable, a declined finding and a scope-expanding fix (operator input), expanding and oscillating finding sets, repeated failed attempts, interrupted-attempt recovery, an already-held candidate lock, an attempted reviewer mutation, a host without fresh-subagent support, a bound source-binding's ahead/behind reporting, and a regression test reproducing (and proving fixed) a stale-disposition defect a first review pass found.SKILL.mdto document the new entry point and records today's changelog entry.Review history
Two full fresh-subagent
review-code-changepasses:498983d):changes_required— oneblockingcorrectness finding (a resumed invocation could leave aconvergedresult carrying both a staledeclineddisposition and the realselected/fixed one for the same finding_id, reachable through this module's own publicresume_checkpointparameter) and onestrong_recommendationfinding (alocal_commitinvocation carrying a schema-legalsource_bindingstill reportedsource: unavailableinstead of the terminal-result contract's own named "local ahead/behind state ... when a source is bound").cc9d86a): both findings fixed —_State.record_finding_dispositionnow always replaces any prior entry for a finding_id before appending a new one;_checkpoint_source_status/_terminal_source_statusnow compute real ahead/behind counts viagit rev-list --countwhenever a source binding is present. Two regression tests added reproducing each defect directly.cc9d86a):clean. Both fixes independently verified; solution-simplicity and code-simplicity also run fresh. One non-gatingdeferfinding remains (the two source-status builders share near-duplicate ahead/behind computation) — preserved per the review-and-merge-gates instruction to keep deferred findings without expanding the PR; noted for a future pass.Acceptance ledger (ticket #99)
convergedonly after a complete clean re-review and required validationFixCycleTests,_check_converged_requires_clean_evidenceinvalidate.pyenforced on every returned documentOperatorInputTests.test_declined_finding_blocks_on_operator_input_and_stays_visible,RegressionTestsBudgetExhaustionTests,ValidationFailureTests,RecoveryTests,OperatorInputTests,StopConditionTestslocal_commitpathrun_local_commitnever callsgit push/any remote-mutating command;publication.statusis alwaysnot_applicableBudgetExhaustionTests,StopConditionTests(unpushed_commitsalways populated)run_local_commitAPI with only the three documented host-boundary ports, no caller-specific stateValidation
python3 -m unittest discover -s skills/review-fix-loop/scripts/tests -p 'test_*.py'— 253 tests, OKjust format— no changes neededjust lint— All checks passed (ruff, mdformat, skills-ref validate, plugin packaging)just test— full repository gate, OK (implement-epic 17, implement-ticket 88, review-code-change 9, review-code-simplicity 3, review-correctness 8, review-fix-loop 253, review-solution-simplicity 5, review-suite 318)Fixes #99
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com